Hash: Refresh for external use. by SolidWallOfCode · Pull Request #4481 · apache/trafficserver

SolidWallOfCode · 2018-10-24T14:17:47Z

This updates the API for hash functions in ATS to make it suitable for external (plugin) use.
The basic hashes are moved to the extern C++ area. The more obscure hashes are kept in core, and MD5 isn't exported because that would create a dependency on the openSSL library.

Also, I need this for the YAML work.
Depends on #4480.

ywkaras · 2018-10-24T14:50:25Z

include/tscore/ConsistentHash.h

@@ -46,17 +47,17 @@ typedef std::map<uint64_t, ATSConsistentHashNode *>::iterator ATSConsistentHashI
 */

 struct ATSConsistentHash {


As long as you're in here, why is this a struct not a class?

You'd have to ask @jrushford - I wanted to minimize the changes in this area, it's really outside the scope of the PR.

ywkaras · 2018-10-24T15:06:30Z

include/tscpp/util/Hash.h

+  virtual self_type &update(std::string_view const &data) = 0;
+
+  /// Finalize the hash function output.
+  virtual self_type & final() = 0;


Interesting. Although final is a keyword, it's not a reserve word. Consistency is the hobgoblin of little programming languages.

We could change that, it's a legacy from pre-eleventy days.

ywkaras · 2018-10-24T15:16:06Z

include/tscpp/util/Hash.h

+};
+
+/// A hash function that returns a 32 bit result.
+struct Hash32Functor : HashFunctor {


OK I looked it up structures, unlike classes, have public inheritance by default. Or, you could say public and people would not have to look it up.

That's an interview question - you shouldn't have to look it up. It's less obscure than "final" being a keyword.

Haha too late already hired me.

I like struct for that exact reason, it instantly tells me that it's all public, and I don't think we should discourage the use of struct over class.

@zwoop I was referring to using default access control for inheritance. I prefer it explicit, unlike SolidWallOfObscurity.

I made another comment above, questioning the use of struct instead of class, when the class has some members with private or protected access control.

In this particular case, it's inertia. When I write new code, I tend to follow Leif's approach and only use struct for POD things.

ywkaras · 2018-10-24T15:18:58Z

include/tscpp/util/Hash.h

+ * The main purpose of this is to allow run time changes in hashing, which is required in various
+ * circumstances.
+ */
+struct HashFunctor {


Aren't functors supposed to have a function call operator?

I'm open to suggestions for a better name. I thought "Hash" was too generic. OTOH the point of this is to embed a function, which makes it a functor.

HashBase? HashSuper? HashSmoker?

"HashFunction", except you'd complain it wasn't a function. "HashFunctionContainer" :-). "HashWrap".

HashAbstract

HashFunctorish
HashFunctoresque
HashFunkyFunctor
HashFlunktor

ywkaras · 2018-10-24T15:28:20Z

include/tscpp/util/Hash.h

+  virtual size_t size() const = 0;
+
+  /// Copy the result to @a dst.
+  /// @a dst must be at least @c result_size bytes long.


result_size https://st.depositphotos.com/1862799/1578/i/950/depositphotos_15785923-stock-photo-dog-puzzled.jpg

Artifact from some API reshuffling.

ywkaras · 2018-10-24T15:58:01Z

include/tscpp/util/Hash.h

+
+  /// Copy the result to @a dst.
+  /// @a dst must be at least @c result_size bytes long.
+  /// @return @c true if the result was copied to @a data, @c false otherwise.


s/data/dst/
?

SolidWallOfCode · 2018-10-24T16:20:20Z

I shuffled the API around quite a bit working on this, I need to review the comments and make sure they're consistent with the final API.

ywkaras · 2018-10-24T16:53:05Z

include/tscpp/util/HashFNV.h

+
+  template <typename X, typename V> self_type &update(TransformView<X, V> view);
+
+  template <typename X, typename V> value_type hash_immediate(TransformView<X, V> const &view);


Why does this take a reference rather than a value param like update()?

Because update uses the parameter as local variable and hash_immediate doesn't. It shouldn't matter from a caller's point of view.

ywkaras · 2018-10-24T19:58:51Z

include/tscpp/util/TextView.h

+
+  /// Check if view is empty.
+  bool empty() const;
+  /// Check if bool is not empty.


s/bool/view/
?

…iews.

This updates the API for hash functions in ATS to make it suitable for external (plugin) use. The basic hashes are moved to the extern C++ area.

SolidWallOfCode added Plugins Core labels Oct 24, 2018

SolidWallOfCode added this to the 9.0.0 milestone Oct 24, 2018

SolidWallOfCode self-assigned this Oct 24, 2018

SolidWallOfCode requested a review from gtenev October 24, 2018 14:17

SolidWallOfCode mentioned this pull request Oct 24, 2018

Hash: Add hash_immediate to generic hash and FNV hashes. #4223

Closed

ywkaras reviewed Oct 24, 2018

View reviewed changes

bryancall added Incompatible and removed Incompatible labels Oct 24, 2018

ywkaras reviewed Oct 24, 2018

View reviewed changes

include/tscpp/util/TextView.h

/// Check if view is empty.

bool empty() const;

/// Check if bool is not empty.

Copy link

Contributor

ywkaras Oct 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/bool/view/
?

TextView: Add TransformView for doing in place transforms on string v…

4ef7379

…iews.

SolidWallOfCode force-pushed the hash-function-refresh branch 2 times, most recently from 98bd959 to b318330 Compare October 31, 2018 20:14

Hash: Refresh for external use.

b70c4d7

This updates the API for hash functions in ATS to make it suitable for external (plugin) use. The basic hashes are moved to the extern C++ area.

SolidWallOfCode force-pushed the hash-function-refresh branch from b318330 to b70c4d7 Compare October 31, 2018 20:49

SolidWallOfCode closed this Nov 2, 2018

zwoop removed this from the 9.0.0 milestone Jan 7, 2019

		@@ -46,17 +47,17 @@ typedef std::map<uint64_t, ATSConsistentHashNode *>::iterator ATSConsistentHashI
		*/

		struct ATSConsistentHash {


		template <typename X, typename V> self_type &update(TransformView<X, V> view);

		template <typename X, typename V> value_type hash_immediate(TransformView<X, V> const &view);

Conversation

SolidWallOfCode commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SolidWallOfCode commented Oct 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

SolidWallOfCode commented Oct 24, 2018 •

edited

Loading